Skip to content

Render NaN default argument as numpy.nan #2243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Render NaN default argument as numpy.nan #2243

wants to merge 4 commits into from

Conversation

sizmailov
Copy link
Contributor

@sizmailov sizmailov commented Jun 5, 2020 •
edited
Loading

This PR changes how nan values are rendered in docstring.

this PR master 
foo(x: float = numpy.nan) -> None
 
foo(x: float = nan) -> None

Implemented via float overloads for py::arg_v. Might be considered as an overkill for such a small change. On the other hand nan values are quite common sentinel values so it might be worth it to generate more toolable docstrings.

compare.cpp 
#include "pybind11/pybind11.h"
#include "pybind11/embed.h"

namespace py = pybind11;

PYBIND11_EMBEDDED_MODULE(example, m) {
  m.def("foo", [](float x) {}, py::arg("x")=std::numeric_limits<float>::quiet_NaN()); 
}

int main() {
  py::scoped_interpreter guard{};
  py::exec(R"(
      from example import * 
      print(foo.__doc__, end="") 
)");
}

sizmailov added 2 commits June 5, 2020 19:33
@sizmailov
Render NaN default argument as numpy.nan
e76c308 
Example: generated signature changes from
    foo(x: float = nan)
to
    foo(x: float = numpy.nan)
@sizmailov
Disable NaN render as numpy.nan for older compilers
a30af8f 
std::isnan is not available on gcc-4.8
@sizmailov sizmailov force-pushed the doc-gen-nan-default-arg branch from 9136bac to a30af8f Compare June 6, 2020 07:53
@sizmailov
Copy link
Contributor Author

sizmailov commented Jun 6, 2020 •
edited
Loading

__cplusplus >= 201103L doesn't mean the library is c++-11 complaint in gcc-4.8 (std::isnan is missing).
To avoid mess with other compilers versions I've enabled it starting from c++14.

@sizmailov sizmailov force-pushed the doc-gen-nan-default-arg branch from 54fd417 to d4784d1 Compare June 6, 2020 08:22
@sizmailov
Copy link
Contributor Author

On second thought I think I chose wrong place inject the change.

The underlying bigger problem is __repr__ function is not intended to produce valid python expressions, therefore we get malformed function signatures quite often. Probably next suggestion is better candidates to format value for signatures.

pybind11/include/pybind11/pybind11.h

Lines 210 to 213 in eeb1044

if (a.descr)
a.descr = strdup(a.descr);
else if (a.value)
a.descr = strdup(a.value.attr("__repr__")().cast<std::string>().c_str());

For example new extension point can be introduced with something like

tempate<typename T>
struct value_formatter {
   value_formatter(const T& value);
   const char* operator() const;
}

// and later in pybind11.h

if (a.descr)
    a.descr = strdup(a.descr);
else if (a.value)
    a.descr = strdup(value_formatter(cast(a.value))());

@sizmailov sizmailov marked this pull request as draft June 10, 2020 01:48
@wjakob
Copy link
Member

wjakob commented Jun 10, 2020

I'm curious: what's wrong with just nan? I don't think numpy.nan is a good alternative, pybind11 is used in all sorts of places that don't even have numpy installed.

@sizmailov
Copy link
Contributor Author

The problem is that generated signatures are not valid in sense of interpretation as stubs signatures. nan refers to a name which is not defined, so docgen/static analysis tools gets confused. The numpy.nan can be replaced with float('nan'), but I think it's a bit more readable.

The situation is worse when default value is not even valid python expression, e.g. x: Foo = <Foo instance 0xffff>. I think it needs generic solution so I close this PR. In shed of light #2244 closing I think you will argue against any PRs in this direction too.

Documentation matters therefore I think it's worth to produce toolable docstrings.

@sizmailov sizmailov closed this Jun 10, 2020
@sizmailov sizmailov mentioned this pull request Nov 17, 2020
Closed
@rwgk rwgk mentioned this pull request Feb 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sizmailov @wjakob